-
Notifications
You must be signed in to change notification settings - Fork 479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[kong] add RBAC resources for controller 2.x #364
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM and I don't have any blockers, but I do want to point out one consideration: can/should we temporarily hide these behind a flag (like a feature gate) in the next
branch until we officially commit to KIC 2.x support in the coming weeks?
Remind me if you would please, what are we blocked on here? |
Originally it was finishing up Kong/kubernetes-ingress-controller#1215 to determine the actual permissions needed. I'd further wanted to finish Kong/kubernetes-ingress-controller#1256 to pull RBAC content from the complete manifests instead of directly from the kubebuilder-generated stuff. https:/Kong/kubernetes-ingress-controller/blob/next/railgun/config/rbac/leader_election_role.yaml and the ClusterRole in https:/Kong/kubernetes-ingress-controller/blob/next/railgun/config/rbac/role.yaml (alternate version: https:/Kong/kubernetes-ingress-controller/blob/9813575912655554c4d5d823281f428464034a62/deploy/single-v2/all-in-one-dbless.yaml#L1005-L1250) are now correct. However, they don't include the finalizer permissions that alpha.2 expects; they'll only work with a next image. We should be good to go ahead and add in the new RBAC resources and do another pass to compare them against 1.x--not sure if we'd want to try and replace the original roles entirely or do what I'd originally done here, which is to install both roles. They should be closer (if not identical) now. |
Did some manual review of the new resources now that we've adjusted permissions and manifest script is ready. As best I can tell, the 2.x permissions are a superset of the 1.x permissions, and as such we can simply replace the 1.x permissions in the chart entirely. I don't think the additional permissions requested (mostly status permissions and new CRDs) are likely to be a concern for security-conscious users. There are a few caveats, however. I had to perform this comparison manually, as the permission generator for 2.x generates a rule per resource, and the 1.x permissions grouped them. For 1.x, you'll have something like this:
whereas 2.x will have:
So they're functionally identical, but you can't diff them. I don't know of a tool that will calculate effective permissions, but the list is small enough that it was easy enough to walk. There were some other differences:
|
I have searched around and I was unable to verify whether these are equivalent on initial inspection, we'll need to double check this.
Sounds good to me 👍
This does indicate a specific compatibility issue with |
Closing in favor of #419, which was written from scratch with a different approach and updated permissions. It does not add new role templates, and instead only replaces role permissions.
Not sure I follow the exact issue. The difference in names is basically arbitrary--we set The chart can't use the original roles verbatim because it templates them to include the release name. It edited the names for v1 roles and such also, and should continue to do so with v2. |
What this PR does / why we need it:
From https:/Kong/kubernetes-ingress-controller/tree/a556daddd55f164bd1d327611c541e8af35f2251, take
kustomize build
output fromrailgun/config/rbac
, replace names, roleRefs, and subjects with template values used by the chart, and append that to the controller RBAC template.Which issue this PR fixes
Related to Kong/kubernetes-ingress-controller#1352
Special notes for your reviewer:
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
next
branch and targetsnext
, notmain
[kong]
)